Add director notfiy method and use it for admin health changes #4186
Conversation
|
Your branch didn't start on the tip of master and conflicts with the change I both suggested and pushed after verifying my suspicion in a test case. The "problem" with this change is the ABI breakage it introduces (the real problem being of course the bug you are trying to fix). edit: it's probably fine to include as part of the 7.6 series with VRT 20.0, despite the 7.6.0 release |
Yes, I noticed, will adjust.
I think this change would call for a |
nigoroll
force-pushed
the
vdi_notify
branch
2 times, most recently
from
fe350b8 to
9a4e6ea
Compare
Absolutely not, this is changing the layout of |
nigoroll
force-pushed
the
vdi_notify
branch
from
9a4e6ea to
a217715
Compare
You are right. More specifically, a member is added, so |
nigoroll
force-pushed
the
vdi_notify
branch
from
a217715 to
6e48ee3
Compare
|
This causes a panic on a simple VTC, see #4189 |
nigoroll
force-pushed
the
vdi_notify
branch
from
6e48ee3 to
57e2a64
Compare
|
OKed during bugwash |
| static void | ||
| vbe_connwait_signal_all(const struct backend *bp) |
There was a problem hiding this comment.
Suggestion: vbe_connwait_broadcast()
| .destroy = vbe_destroy, | ||
| .panic = vbe_panic, | ||
| .list = vbe_list, | ||
| .healthy = vbe_healthy | ||
| .healthy = vbe_healthy, | ||
| .notify = vbe_notify |
There was a problem hiding this comment.
I was bothered by the notify method because this is a breaking change as we established earlier and I wasn't satisfied with such a resolution on the 7.6 branch where we'd rather not break VRT if we can help it.
It then occurred to me that in spirit the new notify method is very close to the event method and I was wondering whether we'd agree to the creation of a new going-sick event even though that was not the original scope of the event enum.
I don't personally see a strong reason against widening the scope beyond the existing set of events.
There was a problem hiding this comment.
Is that yes, no or something else, @dridi ?
There was a problem hiding this comment.
Ah, phk also brought up using the .event method, is this what you mean? I can look at it, but it did not seem natural to me.
There was a problem hiding this comment.
I was suggesting adding a new enum entry for the event callback, one that would be dedicated to backend/directors, instead of adding a new notify callback.
There was a problem hiding this comment.
FTR: I did consider using the .event() method, but it seemed confusing to me because it is tied to the VCL temperature / lifetime. But as both you and phk do not share this concern, I will implement this to have something to look at.
Pondering solutions for varnishcache#4183 it became apparent that we currently have no way for core code to notify directors of changes, in particular changes of the admin_health. I looked into re-using the healthy callback, but it is unsuitable because VRT_Healthy() does not even call it for an "auto" admin_health. So the cleanest option is another director callback, which is cheap and also helps us maintain clean layering.
Using the new facility, we can now selectively notify interested backend types (so far: VBE only) when the admin health changes. This fixes the layer violation introduced with e46f972, where a director private pointer was used with a VBE specific function. Fixes varnishcache#4183
but also it does not need to be notified Fixes varnishcache#4189
nigoroll
force-pushed
the
vdi_notify
branch
from
57e2a64 to
280450a
Compare
|
The alternative implementation with a |
|
obsoleted by #4196 |
Pondering solutions for #4183 it became apparent that we currently have no way for core code to notify directors of changes, in particular changes of the
admin_health. I looked into re-using the healthy callback (see #4184 for my failed attempt), but it is unsuitable becauseVRT_Healthy()rightly does not even call it for an "auto" admin_health.So the cleanest option is another director callback, which is cheap and also helps us maintain clean layering.
This fixes the layer violation introduced with e46f972, where a director private pointer was used with a VBE specific function.
Fixes #4183